Repository navigation
Add Axum production startup and bounded shutdown - #406
ChristianPavilonis wants to merge 7 commits into
Conversation
3ab2209 to
49ba021
Compare
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
Adds explicit production startup to the Axum runner: an app-owned initializer, startup-only typed state, portable probe handlers, and draining on SIGINT/SIGTERM within a fixed time budget. Requesting changes for three reasons:
- The subprocess suite that tests this behavior never runs in CI, and it fails intermittently in parallel.
- Two existing paths regress: embedding installs a global logger, and dev startup no longer shows its URL or the real cause of a bind failure.
- The user docs no longer match the dev behavior.
Findings
Blocking
-
🔧 Subprocess suite never runs in CI (
production_hosting.rs:2) -
🔧 Port reservation race; 4 of 9 parallel runs failed locally (
production_hosting.rs:60) -
🔧 Embedding installs a global logger; standalone installs it after
App::build(dev_server.rs:1014) -
🔧 Dev startup lost the listening URL and mislabels bind failures (
dev_server.rs:473,:654) -
🔧 Axum guide no longer matches dev behavior:
docs/guide/adapters/axum.md:144-146says shutdown "closes the owned connections rather than waiting indefinitely for a graceful drain".run_appandAxumDevServer::runnow:- drain for up to
EDGEZERO__ADAPTER__SHUTDOWN_GRACE_SECONDS(default 10); - handle SIGTERM, and a second signal forces exit;
- fail dev startup on an invalid grace value.
The variable table at
axum.md:105doesn't list the setting. None of the new public API has a guide entry:run_production_app*,run_app_with_options*,AxumRunOptions,PreparedStores,probe::{readiness, liveness},App::insert_state,AppConfig::from_binding. At minimum, update the shutdown paragraph and the variable table in this PR. - drain for up to
-
❓ Should this PR close #392 and #396? The body says
Closes #392/Closes #396, but it also lists remaining acceptance work: production adoption/storage guides, standalone release and Docker UID evidence, unreadable/unusable mounts, and retained-handle lock evidence. The design doc (line 427) says this work "does not complete all acceptance criteria for #392 or #396". Once #275 merges and this PR is retargeted tomain, merging it will auto-close both issues. Should these bePart of #392/Part of #396until the remaining items land?
Non-blocking
- 🤔 Existing
AxumDevServerpaths now take over SIGTERM;run()grace is hard-coded (dev_server.rs:507) - ♻️ Drain vs cancel is inferred from how the listener was obtained (
dev_server.rs:610) - ♻️ Config failure categories depend on message strings (
dev_server.rs:1080) - 🤔 Every KV open failure reads the same (
key_value_store.rs:166) - ♻️ Duplicate bare-handle normalization (
dev_server.rs:361) - ♻️ A
LifecycleReaderis allocated per request (service.rs:349) - 🤔 Test gaps: a post-exit connect check that can't fail, no subprocess coverage of the dev entrypoint, no test for dev KV alias reuse, logger path untested (
production_hosting.rs:243) - ⛏ Local path and stale status in the design doc (
2026-10-05-axum-production-startup-lifecycle-design.md:10) - 🌱 Mark
LifecyclePhase#[non_exhaustive](probe.rs:19) - ⛏ Config-store limit validation depends on whether a config store is declared (
dev_server.rs:1068)
📌 Out of Scope
- Two changes are unrelated to production hosting: widening the
pull_requesttriggers intest.yml,format.ymlandcodeql.ymlto every base branch (CodeQL now runs on every stacked PR), and the generator image-helper fix (9f090e57). Moving them to their own PRs, or into #275, lets each be reviewed and reverted separately.
CI Status (local, head 9f090e57)
- fmt: PASS
- clippy (
--workspace --all-targets --all-features -D warnings): PASS - tests (
--workspace --all-targets): PASS, butproduction_hosting.rsran 0 tests - feature check (
fastly cloudflare spin): PASS - spin wasm32-wasip2: PASS
- hosting suite (
-p edgezero-adapter-axum --all-features --test production_hosting, not run in CI): 3/3 serial runs PASS; 5/9 parallel runs PASS, 4 FAIL withstartup bind: listener: address unavailable
| @@ -0,0 +1,656 @@ | |||
| //! Real local processes, public entrypoints and ordinary main termination output. | |||
| #![cfg(all(feature = "axum", feature = "test-utils", unix))] | |||
There was a problem hiding this comment.
🔧 This suite never runs in CI
The file is gated on feature = "test-utils", and no workflow step enables axum,test-utils for cargo test. The workspace job compiles it and reports Running tests/production_hosting.rs … 0 passed. The green hosted checks therefore say nothing about signals, draining, startup failures or storage. The WASM compile check for edgezero-core/tests/production_handlers.rs has also only been run locally.
Fix: add a step to the workspace suite in test.yml (in a job with the wasm targets installed), wrapped in scripts/run_test_nonzero.sh like the other targeted steps so a silent 0-test run fails:
- name: Production hosting subprocess acceptance
if: matrix.suite == 'workspace'
run: |
cargo test --locked -p edgezero-adapter-axum --features axum,test-utils --test production_hosting
for target in wasm32-wasip1 wasm32-wasip2 wasm32-unknown-unknown; do
cargo check --locked -p edgezero-core --test production_handlers --target "$target"
doneFix the port race (next comment) first, or this step will fail intermittently.
| .expect("envelope") | ||
| } | ||
|
|
||
| fn address() -> SocketAddr { |
There was a problem hiding this comment.
🔧 Port reservation race makes the suite flaky in parallel
address() binds 127.0.0.1:0, records the port and drops the listener. Before either child binds, the kernel can give the same port to another test's address() call. Locally (macOS) the suite failed 4 of 9 default-parallel runs, each time in a different test (second_signal_forces_before_budget_exhaustion, process_budget_forces_pending_handler_stream_and_zero_read_peer, both_signals_stop_idle_processes, embedding_does_not_install_process_signal_handlers). Every failure was early process failure: Error: startup bind: listener: address unavailable. All 3 --test-threads=1 runs passed. This may be the same thing as the "unclassified native default-parallel deadline misses" in the PR body.
Fix: never hand out the same port twice within the test binary. The restart test's deliberate reuse of its own addr still works:
fn address() -> SocketAddr {
static ISSUED: Mutex<BTreeSet<u16>> = Mutex::new(BTreeSet::new());
loop {
let addr = TcpListener::bind("127.0.0.1:0")
.expect("port reservation")
.local_addr()
.expect("address");
if ISSUED.lock().expect("issued ports").insert(addr.port()) {
return addr;
}
}
}Collisions with other processes are still possible but rare. Serializing the suite is the alternative.
| options.validate_stores(metadata)?; | ||
| let mut app = App::build::<A>(crate::AXUM_PLATFORM) | ||
| .map_err(|_error| anyhow::anyhow!("application configuration failed"))?; | ||
| init_logging::<A>(options.logging_level())?; |
There was a problem hiding this comment.
🔧 Logger installation: embedding installs a global logger, standalone installs it too late
- Embedding.
run_app_with_options*install a process-wideSimpleLoggerunless the app owns logging. A throwaway test that calledrun_app_with_optionstwice in one process (defaultHooks, stop after 100 ms) gotOk(())and thenErr(startup logging: application: initialization failed). A host that has already installed a logger fails the same way on the first call.SimpleLogger::initalso callslog::set_max_levelbeforeset_boxed_loggerfails, so the failed attempt still overwrites the host's level. The embedding variants already avoid process-wide signal handlers, and the logger is process-wide state in the same way. - Standalone. The logger is installed after
App::build, so anythingHooks::configurelogs is dropped, and its failure is reduced to a bareapplication configuration failed, which doesn't use thefailure()format. The dev path installs the logger before app configuration.
The fixture's owns_logging() -> true means neither path is tested.
Fix: install logging only in the standalone variants, before app assembly:
fn run_production_selected<A, Initialize, Stop>(
options: AxumRunOptions,
signals: bool,
stop: Stop,
initialize: Initialize,
) -> anyhow::Result<()>
where /* unchanged */
{
if signals {
init_logging::<A>(options.logging_level())?;
}
run_local(/* … */, async move {
let metadata = A::stores();
options.validate_stores(metadata)?;
let mut app = App::build::<A>(crate::AXUM_PLATFORM)
.map_err(|_error| failure("configure", "application", "assembly failed"))?;
// …
})
}Then document that embedders own their logger, and that with_logging_level applies only to the standalone entrypoints. Alternatively, treat SetLoggerError as success when embedding, without calling set_max_level first.
| fn adopt_listener(source: ListenerSource) -> anyhow::Result<TokioTcpListener> { | ||
| let listener = match source { | ||
| ListenerSource::Bind(addr) => StdTcpListener::bind(addr) | ||
| .map_err(|_error| failure("bind", "listener", "address unavailable"))?, |
There was a problem hiding this comment.
🔧 Dev startup lost the listening URL and mislabels bind failures
Before this PR, run_app logged starting axum server on http://{addr} and failed with failed to bind dev server to {addr}: {io error}. Now it logs native server ready (line 654) without an address. Every bind error becomes address unavailable, including permission denied on a port below 1024 and an IP that isn't local. demo_logging.rs was changed to expect the new text. This affects everyone using edgezero serve or run_app, and a parsed SocketAddr is not secret.
Fix:
ListenerSource::Bind(addr) => StdTcpListener::bind(addr).map_err(|error| {
let category = match error.kind() {
ErrorKind::AddrInUse => "address in use",
ErrorKind::PermissionDenied => "permission denied",
ErrorKind::AddrNotAvailable => "address not available",
_ => "bind failed",
};
failure("bind", "listener", category)
})?,and at line 654:
let bound = listener
.local_addr()
.map_err(|_error| failure("bind", "listener", "local address unavailable"))?;
log::info!("[edgezero] native server ready on http://{bound}");| } | ||
| #[cfg(unix)] | ||
| { | ||
| let interrupt = signal::unix::signal(signal::unix::SignalKind::interrupt()) |
There was a problem hiding this comment.
🤔 Existing AxumDevServer paths now take over SIGTERM
enable_ctrl_c (default true) now also registers a SIGTERM handler. That covers AxumDevServer::run() and the async embedding API serve() (through serve_local → run_local). tokio never unregisters signal handlers, so once a host has called serve(), SIGTERM no longer terminates that process. Before this PR only SIGINT was handled. run() also drains with a hard-coded Duration::from_secs(10) (line 180) that ignores SHUTDOWN_GRACE_SECONDS. The same literal appears at run_options.rs:141 and :302.
Suggest keeping the legacy enable_ctrl_c paths SIGINT-only (or adding a separate field for SIGTERM), plus a single DEFAULT_SHUTDOWN_GRACE constant. If the SIGTERM change is intended, document it on the field and in the Axum guide.
| if let Some(phase) = self.phase.clone() { | ||
| core_request | ||
| .extensions_mut() | ||
| .insert(LifecycleReader::new(move || Some(*phase.borrow()))); |
There was a problem hiding this comment.
♻️ A LifecycleReader is allocated on every request
LifecycleReader::new allocates a new Arc-wrapped closure and clones the watch receiver on every request, including requests that never read it. Build the reader once in with_phase and store it next to the private receiver, which the gate keeps using. Insert a clone here; that's only an Arc reference-count bump.
| ); | ||
| assert!(!log.contains("INITIALIZED")); | ||
| TcpStream::connect_timeout(&addr, Duration::from_millis(50)) | ||
| .expect_err("failed startup never accepts connections"); |
There was a problem hiding this comment.
🤔 Test gaps
- This connect check runs after
server.wait()has seen the child exit, so nothing can be listening and the check can't fail. To show the server never accepts before failing, probe while the process is still starting, asstop_during_startup_never_binds_or_becomes_readydoes. - The fixture's
developmentandbare-devmodes are never used, so no subprocess test coversrun_app's new behavior: SIGTERM handling, draining for up to the grace period, and a second signal forcing exit. - The dev-path reuse of one KV handle for ids that resolve to the same name (
build_kv_registry,dev_server.rs:1161) has no test. A unit test asserting that two such ids share a handle would lock in that fix. - The production
init_loggingpath is never exercised because the fixture owns logging. An embedding case with default logging would have caught the double-install failure.
|
|
||
| Base: [PR #275](https://github.com/stackpop/edgezero/pull/275), inspected at `5527b58e2a7cb6307d5c7a8926cd5dfc6f4f8b4a`. | ||
|
|
||
| Worktree: `/home/pav/worktrees/issue-392-396-production-hosting-spec`. |
There was a problem hiding this comment.
⛏ Stale and local-only details in the design doc
- This line commits a personal local path that means nothing to other readers.
- Line 427 says the base "has since advanced to
dfd439c…" and that integration is still open, while the PR body says the branch is ported to current feat: implement portable outbound HTTP and request lifecycle contracts #275. The Status line (5) and the "Review boundary" section should match the head under review. - The issue links point at
stackpop/edgezero; the repo is nowIABTechLab/edgezero.
|
|
||
| /// Observed native runner phase. Once draining starts, a run never becomes ready again. | ||
| #[derive(Clone, Copy, Debug, Eq, PartialEq)] | ||
| pub enum LifecyclePhase { |
There was a problem hiding this comment.
🌱 Mark LifecyclePhase #[non_exhaustive] while it's new
It's a public enum in the portable crate, and apps that write their own probes will match on it. Adding a phase later (a degraded state, for example) would then be a breaking change. Marking it #[non_exhaustive] now costs nothing.
| .config_dir() | ||
| .ok_or_else(|| failure("settings", "CONFIG_DIR", "required"))?; | ||
| let mut by_id = BTreeMap::new(); | ||
| let limits = ConfigStoreLimits::from_env(options.env_config())?; |
There was a problem hiding this comment.
⛏ Config-store limit validation depends on whether a config store is declared
from_os_vars rejects a non-Unicode EDGEZERO__ADAPTER__CONFIG_STORE__* value for every app (run_options.rs:278). A Unicode value such as abc is parsed only here, inside if let Some(meta) = metadata.config. So a production app with no config stores rejects the first and silently accepts the second. Either parse ConfigStoreLimits::from_env unconditionally next to AxumIngressConfig::from_env in run_production_selected, or drop the CONFIG_STORE arm from the non-Unicode check.
dhruv8sh
left a comment
dhruv8sh
left a comment
There was a problem hiding this comment.
PR Review
Summary
Production hosting for the Axum adapter: bounded draining, startup-time store/config extraction, and redacted startup errors. The design is solid and well tested; two behavioural questions need answers before merge.
😃 Praise
- Moving extraction off
RequestContext(AppConfig::from_binding,router.rsinsert_state) lets startup and requests share the same limits, validation and redaction. Copy-on-write startup state with old-admission invalidation is tidy and covered bystartup_state_forks_router_and_invalidates_old_admission. - Errors stay redacted end to end (single message, no source chain, sentinel tests). KV stores sharing a name now get one handle, which also fixes a redb lock conflict on the dev path. Draining is bounded and a second signal forces a non-success exit.
Findings
Blocking
- ❓ Second embedded run in one process fails (
crates/edgezero-adapter-axum/src/dev_server.rs,run_production_selected→init_logging::<A>): the global logger is installed per run, so a secondrun_app_with_options*call (restart, tests) fails withstartup logging: application: initialization failed. Also, anApp::buildfailure returns a bare "application configuration failed" without thestartup …:prefix. Fix: make logger init idempotent (ignore already-set) or move it out of the embedding API; usefailure("configure", "application", …)for the build error. - ❓ Ctrl-C behaviour change on dev server /
run_app(dev_server.rs,AxumDevServer::runwithDuration::from_secs(10), andrun_app_with_preflight): previously Ctrl-C exited immediately with success; now it drains up to 10s, exits non-zero with "grace exhausted" if a streaming request is open, and also listens for SIGTERM. Not called out forserve/demo. Is this intended? If so document it; otherwise keep cancel semantics (Duration::ZERO) for dev entrypoints.
Non-blocking
- 🤔 Possible busy loop if the phase sender is dropped (
crates/edgezero-adapter-axum/src/connection.rs,drainingfuture): ifreader.changed()returnsErrwhile phase is Ready, the loop spins. Unreachable today sincerun_localholds the sender, but fragile. Fix: onErr, treat as draining. - 🤔 Dev KV path lost diagnostics (
key_value_store.rs,dev_server.rsbuild_kv_registryOptional branch): redaction also applies to dev, so local users no longer see the DB path or underlying error. Fix: redact only in production, or log the source atdebug!. - 🤔 Test fixture ships as a binary in the published crate (
crates/edgezero-adapter-axum/Cargo.toml,[[bin]] edgezero-hosting-fixture): gated byrequired-featuresbut still packaged/installable. Fix:exclude, or move to a test-only crate. - 🤔 Port reservation race in subprocess tests (
crates/edgezero-adapter-axum/tests/production_hosting.rs,address()): binds port 0, drops it, child rebinds — flaky under parallel tests. Fix: retry on bind failure, or have the child report its bound port. - 🤔 Two probe states are unobservable on the native runner (
crates/edgezero-core/src/probe.rs): liveness returns 200 in Starting/Draining, but the listener isn't bound during Starting and dispatch refuses work while Draining. The spec acknowledges it; add one rustdoc line so the table doesn't mislead. - ⛏ Config-store NAME override validated but unused in production (
run_options.rsvalidate_stores,dev_server.rsbuild_production_stores): files are alwayslocal-config-{id}.json. Document or skip validation. - ⛏ File-level
#![expect(clippy::arbitrary_source_item_ordering)]indev_server.rs— scope it to the new items. - ⛏ Leftover string indentation in
crates/edgezero-core/src/extractor.rsresolve_leaf(UnknownStoremessage,[stores.secrets] (id redacted)line). Output unaffected; source only.
📌 Out of Scope
- Removing
pull_request.branches: [main]from.github/workflows/{codeql,format,test}.ymlis a repo-wide CI policy change (needed for stacked PRs, unrelated to hosting). Consider splitting it out;ci_tooling.test.mjsonly coverstest.yml.
CI Status
- fmt: PASS
- clippy: PASS
- tests: PASS (2727 passed, 0 failed)
Current review readiness — 2026-11-10
Ready for review at the publication head below. All29 current hosted checks passed. Reviewers:
aram356,prk-Jr,dhruv8sh.Review transitions are independent of TS work and the combined integration audit; #462 remains draft. This current status supersedes historical draft-only prerequisites below without claiming unexecuted deployment/manual acceptance complete. All three reviewers are requested. Stack465 is the current grouping; #453 is excluded.
Current review-stack checkpoint
Publication head:
9f090e573206cf0d8c21c1ce0067afffd69f12db. This cleanup preserves the existing feature unit and PR identity. The additional change is the completed generated-native build correction: image-only code now follows the scaffold's existing Fastly-example selection, with no request-policy change. Its focused generator/native proof is retained; fresh per-branch hosted checks are not assumed green.Review order: #275 → #406 → #407 → #409 → #414 → #422 → #439 → #454. Draft integration checkpoint: #462. New SDK/group/codec/phase development and unrelated TS work are not prerequisites or additions to this PR. Earlier notes below retain their original tested revisions and limits; they are not a whole-tree pass for this new head.
Earlier implementation and proof notes
RC refresh checkpoint, 2026-11-10
Current head:
1d8e42e1cba52610e6655965e9941f42680b53ab.Current #275 hosting port, with bounded startup/config/ingress and existing callback/egress ownership retained. The invalid-present native-limit and Windows prefix-identity capture bugs found by review are fixed. Windows semantics were simulated at the private parser boundary, not run on Windows.
Fresh CI exposed missing fixture validator metadata, an always-compiled test listener helper and a trigger assertion still expecting main-only PRs. The fixture lock is synced, the helper now has the same test/test-utils gate as its caller, and the exact workflow test now requires all PR bases while retaining main-only push. No lint suppression or production/deployment trigger expansion. Parent proof includes 301 Axum unit tests, axum-only strict Clippy and 35 Node CI tests.
Draft pending fresh hosted checks and the combined RC audit. Two unchanged native default-parallel deadline misses remain recorded and unclassified; serial passes and untouched-foundation parallel passes do not clear them. No timing bound was relaxed. No deployment, main merge, security-alert dismissal or review-thread resolution occurred. Historical design/review context follows.
Summary
This PR is stacked on #275, with base branch
docs/outbound-http-spec. The implementation started at5527b58. It remains draft because #275 has advanced to91f4af38, five parent-only commits beyond the common base5527b58. Parent integration and re-audit, including the previously identifieddev_server.rsconflict, remain outstanding. The recorded checks are for this branch, not integration with the current prerequisite. This relationship audit performed no merge or rebase. Integrate the current contracts and repeat affected validation before marking ready.Changes
edgezero-adapter-axum/src/dev_server.rs,run_options.rsedgezero-core/src/app.rs,router.rsedgezero-core/src/extractor.rs,probe.rsCloses
Closes #392.
Closes #396.
Part of #391. This PR remains draft until the pending integration and acceptance work below is complete.
Remaining acceptance includes production adoption/storage guides, standalone release and Docker UID evidence, unreadable/unusable mount cases, retained-handle lock evidence, and the accepted-#275 re-audit.
Test plan
cargo test --workspace --all-targetscargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkcargo check --workspace --all-targets --features "fastly cloudflare spin"examples/app-demoworkspace:cd examples/app-demo && cargo test --workspace --all-targetscd docs && npm run lint && npm run format && npm run buildedgezero serve --adapter axumcargo test -p edgezero-core --all-targetscargo test -p edgezero-adapter-axum --all-targets --all-features, including all 13 production-hosting subprocess testscargo check -p edgezero-adapter-spin --target wasm32-wasip2 --features spincargo check -p edgezero-core --test production_handlers --target <target>forwasm32-wasip1,wasm32-wasip2, andwasm32-unknown-unknowngit diff --checkChecks passed on this branch, not on a merge with the newer prerequisite. Native subprocess tests use debug fixtures and the current user's UID, not a release container image. Shutdown cancellation is cooperative; blocking application code still requires a supervisor hard-kill boundary.
Checklist
{id}syntax, not:idedgezero_core, nothttpKvRegistry/ConfigRegistry/SecretRegistry